Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(selector-card): Add descriptive variant for radio selector card #468

Merged
merged 1 commit into from
Jun 4, 2024
Merged

feat(selector-card): Add descriptive variant for radio selector card #468

merged 1 commit into from
Jun 4, 2024

Conversation

321gillian
Copy link
Collaborator

Figma Link

✅ Closes: COMUI-2784

@github-actions GitHub Actions
Copy link

Demo will be published at https://apps.inindca.com/common-ui-docs/genesys-webcomponents/feature/COMUI-2784-selector-descriptive

@321gillian 321gillian self-assigned this May 21, 2024
@321gillian 321gillian force-pushed the feature/COMUI-2784-selector-descriptive branch from dbc478d to 4be82c7 Compare May 21, 2024 10:46
daragh-king-genesys
daragh-king-genesys reviewed May 22, 2024
View reviewed changes
Copy link
Collaborator

@daragh-king-genesys daragh-king-genesys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a few nit picks :)

packages/genesys-spark-components/src/components/beta/gux-selector-cards/example.html Outdated Show resolved Hide resolved
...k-components/src/components/beta/gux-selector-cards/gux-selector-card/gux-selector-card.scss Outdated Show resolved Hide resolved
@gavin-everett-genesys
Copy link
Collaborator

Just one small issue see image below,

Screenshot 2024-05-27 at 09 13 43

You can see when hovering over the second span that the text overlaps other text on the frame. I think currently the two spans inside have the same z-index but I dont think that will work for some reason. I am not sure if the second span needs a z-index or maybe it needs to be higher potentially.

@gavin-everett-genesys
Copy link
Collaborator

One last issue I should have picked this up in my first review 🙈,
Screenshot 2024-05-28 at 10 33 36

I think the truncation is not working on the label here as the gux-truncate styling is nested under the .gux-description class so that standard radio button variant cannot access the styling I think just move it from there and all should work.

@321gillian
Copy link
Collaborator Author

One last issue I should have picked this up in my first review 🙈, Screenshot 2024-05-28 at 10 33 36

I think the truncation is not working on the label here as the gux-truncate styling is nested under the .gux-description class so that standard radio button variant cannot access the styling I think just move it from there and all should work.

Gavin, you are the MVP for this. Thanks for spotting that!

@321gillian 321gillian force-pushed the feature/COMUI-2784-selector-descriptive branch from 983322f to 2e364ed Compare May 29, 2024 14:33
@321gillian 321gillian force-pushed the feature/COMUI-2784-selector-descriptive branch from 2e364ed to 0da7871 Compare June 4, 2024 08:22
@321gillian 321gillian merged commit a852550 into main Jun 4, 2024
3 checks passed
@321gillian 321gillian deleted the feature/COMUI-2784-selector-descriptive branch June 4, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavin-everett-genesys gavin-everett-genesys gavin-everett-genesys approved these changes

@MattCheely MattCheely Awaiting requested review from MattCheely

@daragh-king-genesys daragh-king-genesys Awaiting requested review from daragh-king-genesys

Assignees

@321gillian 321gillian

Labels
UX Review Wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@321gillian @gavin-everett-genesys @daragh-king-genesys